Add assignment functionality for cases, including responsible doctor …#61
Conversation
…and assigned nurse handling
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds owner/handler assignment support across layers: DTOs, mapper, service authorization and status logic, repository query, UI form and controller endpoint, templates, tests, and seed data; removes one entity/DTO field and replaces enum member name. Changes
Sequence DiagramsequenceDiagram
participant User as User (Doctor/Manager)
participant UI as UiController
participant CaseService as CaseService
participant EmployeeSvc as EmployeeService
participant Repo as Repository/DB
User->>UI: POST /ui/cases/{id}/assignments (ownerId?, handlerId?)
UI->>CaseService: assignUsers(caseId, CaseAssignmentDTO, actor)
CaseService->>Repo: fetch CaseEntity by id
Repo-->>CaseService: CaseEntity
alt ownerId present
CaseService->>EmployeeSvc: findById(ownerId)
EmployeeSvc->>Repo: query employee
Repo-->>EmployeeSvc: EmployeeEntity
EmployeeSvc-->>CaseService: EmployeeDTO
CaseService->>CaseService: authorize (doctor/manager rules)
end
alt handlerId present
CaseService->>EmployeeSvc: findById(handlerId)
EmployeeSvc->>Repo: query employee
Repo-->>EmployeeSvc: EmployeeEntity
EmployeeSvc-->>CaseService: EmployeeDTO
end
alt status change needed (current != ASSIGNED)
CaseService->>CaseService: set status = ASSIGNED
CaseService->>Repo: save CaseEntity
CaseService->>Repo: record status audit (previous -> ASSIGNED)
else already ASSIGNED
CaseService->>Repo: save CaseEntity (no audit)
end
Repo-->>CaseService: persisted entity
CaseService-->>UI: redirect / case detail
UI-->>User: redirect response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/org/example/projektarendehantering/application/service/CaseService.java (1)
213-220: No mechanism to un-assign owner or handler.The current logic only sets
ownerId/handlerIdwhen the DTO field is non-null. Combined with the controller ignoring empty strings, there's no way to clear an assignment (set back to "Unassigned").If un-assignment is a required feature, consider using a sentinel value or separate clear flags:
♻️ Potential approach to support clearing
One option is to distinguish between "not provided" (null) and "explicitly cleared" (e.g., a dedicated field or accepting empty UUID):
// In CaseAssignmentDTO, add flags: private boolean clearOwnerId; private boolean clearHandlerId; // In assignUsers: if (dto.isClearOwnerId()) { entity.setOwnerId(null); } else if (dto.getOwnerId() != null) { // existing validation and set }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/CaseService.java` around lines 213 - 220, The current logic only updates owner/handler when dto.getOwnerId()/dto.getHandlerId() are non-null, so there's no way to clear assignments; add boolean flags (e.g., isClearOwnerId/isClearHandlerId) to the DTO and update the assignment logic in the service: for owner use if (dto.isClearOwnerId()) { entity.setOwnerId(null); } else if (dto.getOwnerId() != null) { UUID ownerId = requireEmployeeWithRole(dto.getOwnerId(), Set.of(Role.DOCTOR), "ownerId"); entity.setOwnerId(ownerId); } and similarly for handler (use isClearHandlerId, requireEmployeeWithRole for Role.NURSE, and entity.setHandlerId(null)/setHandlerId(handlerId)) so clearing is explicit and validation only runs when assigning.src/main/resources/templates/cases/detail.html (1)
38-58: Consider restricting assignment form visibility by role.The assignment form is visible to anyone who can view the case, but
CaseService.assignUsersonly allows managers and doctors to submit assignments. Nurses will see the form but get an authorization error upon submission.Consider conditionally rendering this section based on user role to avoid a confusing UX:
<div th:if="${currentUser.role.name() == 'MANAGER' or currentUser.role.name() == 'DOCTOR'}"> <!-- assignment form here --> </div>This requires passing the current user/role to the model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/cases/detail.html` around lines 38 - 58, The assignment form should be conditionally rendered so only managers and doctors see it; wrap the form in a Thymeleaf condition (e.g., th:if="${currentUser.role.name() == 'MANAGER' or currentUser.role.name() == 'DOCTOR'}") in the template (detail.html) and ensure the controller that prepares the model for this view adds the currentUser or currentUser.role (so the template can evaluate currentUser.role.name()); this aligns the UI with the CaseService.assignUsers authorization and prevents nurses from seeing the submit form that would produce an auth error.src/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java (1)
500-515: Test coverage gap: status transitions to HANDLER_ASSIGNED even without a handler.This test correctly verifies the status change when a handler is assigned. However, the current
assignUsersimplementation (lines 223-228 in CaseService) transitions toHANDLER_ASSIGNEDwhenever the previous status isn't alreadyHANDLER_ASSIGNED, regardless of whether a handler is actually being set.Consider adding a test case that verifies the expected behavior when only
ownerIdis provided (no handler):`@Test` void assignUsers_shouldNotChangeStatusWhenOnlyOwnerAssigned() { caseEntity.setStatus(CaseStatus.CREATED); CaseAssignmentDTO dto = new CaseAssignmentDTO(); dto.setOwnerId(doctorActor.userId()); // No handlerId // ... verify status remains CREATED or changes appropriately }If transitioning to
HANDLER_ASSIGNEDwithout a handler is unintended, see the related comment onCaseService.java.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java` around lines 500 - 515, The current assignUsers behavior allows CaseStatus to move to HANDLER_ASSIGNED even when no handler is provided; add a unit test named assignUsers_shouldNotChangeStatusWhenOnlyOwnerAssigned that sets caseEntity.status to CREATED, builds a CaseAssignmentDTO with only ownerId (no handlerId), mocks repositories similarly to the existing test, calls CaseService.assignUsers, and asserts the status remains CREATED (and verify auditService not recording a CREATED -> HANDLER_ASSIGNED transition); if behavior is unintended, modify CaseService.assignUsers to only set CaseStatus.HANDLER_ASSIGNED when dto.getHandlerId() is present (and resolution logic still saves/maps/audits consistently).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Around line 222-230: The code unconditionally sets CaseStatus.HANDLER_ASSIGNED
when previousStatus != HANDLER_ASSIGNED even if no handler was assigned; update
the branch in CaseService so you only change status to
CaseStatus.HANDLER_ASSIGNED (and call recordStatusChange/save) when a handler is
actually present or newly assigned on the CaseEntity (e.g., check
entity.getHandler() != null or compare the handler field for change) and keep
assignments of owner/doctor from triggering this path; use the same
savedEntity/caseRepository.save and recordStatusChange calls when the handler
condition is met and otherwise persist without changing status.
In
`@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java`:
- Around line 58-63: The new findByRole method lacks the manager authorization
present in getEmployee and getAllEmployees; update findByRole to accept an Actor
parameter and call requireCanManageEmployees(actor) before querying, or if
intended to be public, add a clear Javadoc explaining that omission (reference
methods findByRole, getEmployee, getAllEmployees and the authorization helper
requireCanManageEmployees to locate the logic to mirror).
In
`@src/main/java/org/example/projektarendehantering/presentation/web/UiController.java`:
- Around line 140-145: In UiController, avoid unhandled IllegalArgumentException
from UUID.fromString in the ownerId/handlerId parsing: wrap the calls that set
dto.setOwnerId(UUID.fromString(ownerId)) and
dto.setHandlerId(UUID.fromString(handlerId)) in try-catch blocks (catch
IllegalArgumentException) and handle malformed UUIDs by returning a 400 Bad
Request (or throwing a WebApplicationException/ResponseStatusException with a
clear message) instead of letting a 500 surface; ensure the error response
identifies which parameter is invalid (ownerId or handlerId) so clients can
correct the request.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Around line 213-220: The current logic only updates owner/handler when
dto.getOwnerId()/dto.getHandlerId() are non-null, so there's no way to clear
assignments; add boolean flags (e.g., isClearOwnerId/isClearHandlerId) to the
DTO and update the assignment logic in the service: for owner use if
(dto.isClearOwnerId()) { entity.setOwnerId(null); } else if (dto.getOwnerId() !=
null) { UUID ownerId = requireEmployeeWithRole(dto.getOwnerId(),
Set.of(Role.DOCTOR), "ownerId"); entity.setOwnerId(ownerId); } and similarly for
handler (use isClearHandlerId, requireEmployeeWithRole for Role.NURSE, and
entity.setHandlerId(null)/setHandlerId(handlerId)) so clearing is explicit and
validation only runs when assigning.
In `@src/main/resources/templates/cases/detail.html`:
- Around line 38-58: The assignment form should be conditionally rendered so
only managers and doctors see it; wrap the form in a Thymeleaf condition (e.g.,
th:if="${currentUser.role.name() == 'MANAGER' or currentUser.role.name() ==
'DOCTOR'}") in the template (detail.html) and ensure the controller that
prepares the model for this view adds the currentUser or currentUser.role (so
the template can evaluate currentUser.role.name()); this aligns the UI with the
CaseService.assignUsers authorization and prevents nurses from seeing the submit
form that would produce an auth error.
In
`@src/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java`:
- Around line 500-515: The current assignUsers behavior allows CaseStatus to
move to HANDLER_ASSIGNED even when no handler is provided; add a unit test named
assignUsers_shouldNotChangeStatusWhenOnlyOwnerAssigned that sets
caseEntity.status to CREATED, builds a CaseAssignmentDTO with only ownerId (no
handlerId), mocks repositories similarly to the existing test, calls
CaseService.assignUsers, and asserts the status remains CREATED (and verify
auditService not recording a CREATED -> HANDLER_ASSIGNED transition); if
behavior is unintended, modify CaseService.assignUsers to only set
CaseStatus.HANDLER_ASSIGNED when dto.getHandlerId() is present (and resolution
logic still saves/maps/audits consistently).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49cd086a-16a9-45ec-9487-04455901fe77
📒 Files selected for processing (8)
src/main/java/org/example/projektarendehantering/application/service/CaseMapper.javasrc/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/application/service/EmployeeService.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeRepository.javasrc/main/java/org/example/projektarendehantering/presentation/dto/CaseDTO.javasrc/main/java/org/example/projektarendehantering/presentation/web/UiController.javasrc/main/resources/templates/cases/detail.htmlsrc/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java
…to `ASSIGNED`, enhance UUID validation in `UiController`, and adjust test and SQL seed data accordingly
|
Ser bra ut, en liten grej att ta bort här bara:
|
Har noterat detta också, inte helt klar på varför den finns med. Tar bort det fältet! |
…epository classes
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/projektarendehantering/application/service/CaseService.java (1)
213-230:⚠️ Potential issue | 🟠 MajorReject empty assignment submissions before flipping the case to
ASSIGNED.Lines 213-230 currently treat a blank form post as a real assignment. The controller converts empty
ownerId/handlerIdinputs tonull, but this block still saves the case and recordsCREATED -> ASSIGNED, even when nobody was assigned.Suggested fix
+ if (dto.getOwnerId() == null && dto.getHandlerId() == null) { + throw new BadRequestException("At least one assignment is required"); + } + if (dto.getOwnerId() != null) { UUID ownerId = requireEmployeeWithRole(dto.getOwnerId(), Set.of(Role.DOCTOR), "ownerId"); entity.setOwnerId(ownerId); } if (dto.getHandlerId() != null) { UUID handlerId = requireEmployeeWithRole(dto.getHandlerId(), Set.of(Role.NURSE), "handlerId"); entity.setHandlerId(handlerId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/CaseService.java` around lines 213 - 230, The current logic in CaseService (around the block that reads dto, sets entity owner/handler, checks previousStatus, sets CaseStatus.ASSIGNED, saves and calls recordStatusChange) flips CREATED->ASSIGNED even when the incoming dto has both ownerId and handlerId null; change it so you first detect an empty assignment submission (dto.getOwnerId() == null && dto.getHandlerId() == null) and in that case do not change the status or call recordStatusChange — simply save and return the entity via caseRepository.save(entity) and caseMapper.toDTO(saved) (or alternatively reject with validation if desired); otherwise proceed with setting status to ASSIGNED, saving, and recording the status change as currently implemented.
♻️ Duplicate comments (1)
src/main/java/org/example/projektarendehantering/presentation/web/UiController.java (1)
140-148:⚠️ Potential issue | 🟡 MinorPreserve the invalid parameter name in the 400 response.
Lines 140-148 now catch malformed UUIDs, but
"Invalid UUID format"still hides whetherownerIdorhandlerIdwas bad. That makes the form/API error harder to correct.Suggested fix
- try { - if (ownerId != null && !ownerId.isBlank()) { - dto.setOwnerId(UUID.fromString(ownerId)); - } - if (handlerId != null && !handlerId.isBlank()) { - dto.setHandlerId(UUID.fromString(handlerId)); - } - } catch (IllegalArgumentException e) { - throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid UUID format"); - } + if (ownerId != null && !ownerId.isBlank()) { + try { + dto.setOwnerId(UUID.fromString(ownerId)); + } catch (IllegalArgumentException e) { + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid ownerId UUID"); + } + } + if (handlerId != null && !handlerId.isBlank()) { + try { + dto.setHandlerId(UUID.fromString(handlerId)); + } catch (IllegalArgumentException e) { + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid handlerId UUID"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/UiController.java` around lines 140 - 148, The catch block in UiController that wraps dto.setOwnerId(UUID.fromString(ownerId)) and dto.setHandlerId(UUID.fromString(handlerId)) should return which parameter caused the bad UUID; change the logic to validate/parse ownerId and handlerId separately (or catch exceptions per parse) and throw a ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid UUID format for ownerId") or "... for handlerId" as appropriate so the 400 message preserves the invalid parameter name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/projektarendehantering/presentation/web/UiController.java`:
- Around line 89-93: The current UiController case-detail handling uses
caseService.getCase(...).ifPresent(...) and falls through to rendering
"cases/detail" even when the Optional is empty; change this to explicitly return
404 when the case is missing by replacing the ifPresent usage with an explicit
presence check or orElseThrow that throws a
ResponseStatusException(HttpStatus.NOT_FOUND) (or otherwise set the response to
404) before adding model attributes (keep the model population code:
model.addAttribute("case", ...), "doctors", "nurses") so the view is only
rendered when the case exists; update the logic around
caseService.getCase(securityActorAdapter.currentUser(), caseId) in UiController
accordingly.
---
Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Around line 213-230: The current logic in CaseService (around the block that
reads dto, sets entity owner/handler, checks previousStatus, sets
CaseStatus.ASSIGNED, saves and calls recordStatusChange) flips CREATED->ASSIGNED
even when the incoming dto has both ownerId and handlerId null; change it so you
first detect an empty assignment submission (dto.getOwnerId() == null &&
dto.getHandlerId() == null) and in that case do not change the status or call
recordStatusChange — simply save and return the entity via
caseRepository.save(entity) and caseMapper.toDTO(saved) (or alternatively reject
with validation if desired); otherwise proceed with setting status to ASSIGNED,
saving, and recording the status change as currently implemented.
---
Duplicate comments:
In
`@src/main/java/org/example/projektarendehantering/presentation/web/UiController.java`:
- Around line 140-148: The catch block in UiController that wraps
dto.setOwnerId(UUID.fromString(ownerId)) and
dto.setHandlerId(UUID.fromString(handlerId)) should return which parameter
caused the bad UUID; change the logic to validate/parse ownerId and handlerId
separately (or catch exceptions per parse) and throw a
ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid UUID format for
ownerId") or "... for handlerId" as appropriate so the 400 message preserves the
invalid parameter name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b12a570f-ba7c-4ad4-8b77-f65092b336dd
📒 Files selected for processing (9)
src/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/common/CaseStatus.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseRepository.javasrc/main/java/org/example/projektarendehantering/presentation/dto/CaseAssignmentDTO.javasrc/main/java/org/example/projektarendehantering/presentation/web/UiController.javasrc/main/resources/data.sqlsrc/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.javasrc/test/resources/data-test.sql
💤 Files with no reviewable changes (3)
- src/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseEntity.java
- src/main/java/org/example/projektarendehantering/presentation/dto/CaseAssignmentDTO.java
- src/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseRepository.java
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/data.sql
…and assigned nurse handling
#42
Summary by CodeRabbit
New Features
Behavior Changes